Reviewed 2 of 15 files at r1, all commit messages.
Reviewable status: 2 of 15 files reviewed, 3 unresolved discussions (waiting on @mojganii)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 94 at r1 (raw file):

        let exitIP: Bool

        enum CodingKeys: String, CodingKey {

This struct already conforms to Codable, so no need to encode or decode manually. This enum, along with init(from decoder: Decoder) and encode(to encoder: Encoder), can be removed.

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 98 at r1 (raw file):

        }

        init(ip: T, exitIP: Bool) {

This init is never used and should be ok to remove.

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 116 at r1 (raw file):

        static func == (
            lhs: OutgoingConnectionProxy.OutgoingConnectionData<T>,

Nit: OutgoingConnectionProxy.OutgoingConnectionData<T> can be substituted with Self instead to increase readability.

Reviewable status: 2 of 15 files reviewed, 3 unresolved discussions (waiting on @mojganii)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 94 at r1 (raw file):

Update: We actually need the mapping enum here, because of exitIP = "mullvad_exit_ip", but the rest is ok to remove.

Reviewable status: 1 of 15 files reviewed, 3 unresolved discussions (waiting on @rablador)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 94 at r1 (raw file):

Done.

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 98 at r1 (raw file):

Done.

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 116 at r1 (raw file):

indeed!

Reviewed 13 of 15 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 67 at r1 (raw file):

    var outAddress: String? {
        let v4 = ipv4.exitIP ? ipv4.ip : nil

This can be compressed a bit, also only adding the line break if there are two items to separate them:

Code snippet:

        let v4: IPAddress? = ipv4.exitIP ? ipv4.ip : nil
        let v6: IPAddress? = ipv6.flatMap { $0.exitIP ? $0.ip : nil }

        let string = [v4, v6].compactMap { $0 }.map { $0.debugDescription }.joined(separator: "\n")
        return string.isEmpty ? nil : string

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 35 at r1 (raw file):

        let result = try await outgoingConnectionService.getOutgoingConnectionInfo()
        if result.ipv4 == .mock,
           let ipv6 = result.ipv6,

Just result.ipv6 == .mock works too.

Reviewed 14 of 15 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii and @rablador)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 91 at r2 (raw file):

    struct OutgoingConnectionData<T: IPAddressType>: Codable, Equatable {
        let ip: T?

Can ip really be nil?

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 67 at r1 (raw file):

@rablador transforming IP to string can be done using "\(ipAddress)", no need to call debugDescription directly.

Reviewable status: 11 of 15 files reviewed, 3 unresolved discussions (waiting on @pronebird and @rablador)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 91 at r2 (raw file):

After checking the server response I figured out it cannot be optional.

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 67 at r1 (raw file):

Done.

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 35 at r1 (raw file):

Done.

That's really good work !

Reviewed 7 of 15 files at r1.
Reviewable status: 11 of 15 files reviewed, 13 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)

ios/MullvadVPN/SceneDelegate.swift line 73 at r2 (raw file):

            accountsProxy: appDelegate.accountsProxy,
            outgoingConnectionService: OutgoingConnectionService(
                outgoingConnectionProxy: OutgoingConnectionProxy(urlSession: .shared)

We don't want to cache the result of calls to am.i.mullvad.net, therefore we don't want to use the .shared sessions.

Please use URLSessionConfiguration.ephemeral instead for getting a URLSession object

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 83 at r2 (raw file):

}

extension OutgoingConnectionProxy {

IMHO OutgoingConnectionData should be in its own file and not part of OutgoingConnectionProxy

The protocol signature would look simpler as well

protocol OutgoingConnectionHandling {
    func getIPV6(retryStrategy: REST.RetryStrategy) async throws -> IPV6ConnectionData
    func getIPV4(retryStrategy: REST.RetryStrategy) async throws -> IPV4ConnectionData
}

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

    func getOutgoingConnectionInfo() async throws -> OutgoingConnectionInfo {
        try await withThrowingTaskGroup(of: OutgoingConnectionResult.self) { taskGroup -> OutgoingConnectionInfo in

We can simplify this.
However, technically we are not running the calls in parallel but that shouldn't matter in practice. Also we're shaving 40 lines. IMHO it's worth it.

func getOutgoingConnectionInfo() async throws -> OutgoingConnectionInfo {
    let ipv4 = try await outgoingConnectionProxy.getIPV4(retryStrategy: .default)
    let ipv6 = try await outgoingConnectionProxy.getIPV6(retryStrategy: .noRetry)

    return OutgoingConnectionInfo(ipv4: ipv4, ipv6: ipv6)
}

I don't think we should be touching the task priority until we have good reason to believe that this would be significantly better for the UX.
If we absolutely need to, then we can change the call site instead (which I still think we shouldn't)

outgoingConnectionTask = Task(priority: .userInitiated) { [weak self] in
    guard let outgoingConnectionInfo = try await self?.outgoingConnectionService
        .getOutgoingConnectionInfo() else {
        return
    }
    await self?.didGetOutGoingAddress?(outgoingConnectionInfo)
}

ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 153 at r2 (raw file):

        if let tunnelRelay = tunnelState.relay {
            connectionPanel.dataSource = ConnectionPanelData(
                inAddress: "\(tunnelRelay.endpoint.ipv4Relay) UDP",

We shouldn't be hardcoding UDP here since it's depending on whether we're using the UDP-over-TCP obfuscation.

Maybe let's add a TODO comment for now that we will be solving this soon.

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 42 at r2 (raw file):

        self.outgoingConnectionService = outgoingConnectionService

        let tunnelObserver = TunnelBlockObserver(

Is there anyway we can do this when we start the tunnel instead of the init ?

ios/MullvadVPNTests/MockURLProtocol.swift line 12 at r2 (raw file):

class MockURLProtocol: URLProtocol {
    static var error: Error?

Same remark as in OutgoingConnectionProxyStub, this should not be static

ios/MullvadVPNTests/OutgoingConnectionProxy+Stub.swift line 12 at r2 (raw file):

struct OutgoingConnectionProxyStub: OutgoingConnectionHandling {
    static var hasError = false

Please let's not use static unless we absolutely have to. It always leads to hard to test designs and unwanted shared states.

Here in particular, it makes it so that all the OutgoingConnectionProxyStub have the same error state which is definitely not what we want when running multiple tests in parallel.

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 18 at r2 (raw file):

    private let encoder: JSONEncoder = REST.Coding.makeJSONEncoder()

    override func setUp() {

You can use setUpWithError() throws and remove the force try as well as the swiftlint:disable commands instead

Remember to also use tearDownWithError() throws as well in that case

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 34 at r2 (raw file):

    func testNoInternetConnection() async throws {
        let failureExpectation = expectation(description: "should fail")
        let successExpectation = expectation(description: "should not succeed")

There are a couple of things we can improve in this test.

• Expectations :
  Because we are using inverted expectations here, when this fails, the message produced will be
  Fulfilled inverted expectation "should not succeed".
  Which is a double negation.

In essence, we should keep the same kind messaging so that the error is easy to read and understand.
In this case, it would make sense to read Fulfilled inverted expectation "Received IPv4".

• Expectation descriptions :
  A message such as "should fail" is not very descriptive. What are we expecting here ? We shouldn't have to read what the code does when a test fail to know what failed.
  A much better message would be "Did not receive IPv4".
  That way, if the test fails, this is what we would see "Asynchronous wait failed [...] with unfulfilled expectations: "Did not receive IPv4".
  Now we know that we expected to not receive IPV4, and that the expectation was not fullfilled, therefore, we did receive an IPv4. We probably know how to fix this.

• Expectation names :
  As important as the message, usually, we try to name the expectation according to what's being expected.
  In this case, a good name would be let noIPv4Expectation = expectation(description: "Did not receive IPv4") for example.

• Catching the error :

Unfortunately, the XCTest API is not well suited for async results, otherwise, I would have said to use XCTAssertThrowsError which is suited for that.

So I wrote a helper method to help with that

extension XCTest {
    func XCTAssertThrowsErrorAsync<T: Sendable>(
        _ expression: @autoclosure () async throws -> T,
        _ message: @autoclosure () -> String = "",
        file: StaticString = #filePath,
        line: UInt = #line,
        _ errorHandler: (_ error: Error) -> Void = { _ in }
    ) async {
        do {
            _ = try await expression()
            XCTFail(message(), file: file, line: line)
        } catch {
            errorHandler(error)
        }
    }
}

Here's what the rewritten test looks like now

func testGetIPv4FailsWhenNotConnected() async throws {
    let noIPv4Expectation = expectation(description: "Did not receive IPv4")
    MockURLProtocol.error = URLError(URLError.notConnectedToInternet)
    MockURLProtocol.requestHandler = nil

    await XCTAssertThrowsErrorAsync(try await outgoingConnectionProxy.getIPV4(retryStrategy: .noRetry)) { error in
        noIPv4Expectation.fulfill()
        XCTAssertEqual((error as? URLError)?.code, .notConnectedToInternet)
    }
    await fulfillment(of: [noIPv4Expectation], timeout: 1)
}

I suggest rewriting the other tests in this file with these improvements in mind

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 54 at r2 (raw file):

}

enum MockNetworkError: Error {

nit
Technically, this is a stub, not a mock

Reviewable status: 11 of 15 files reviewed, 12 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

ios/MullvadVPN/SceneDelegate.swift line 73 at r2 (raw file):

Done.

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

but task group does it parallely.regarding the task priority I think we need to change it for better UX

ios/MullvadVPN/View controllers/Tunnel/TunnelControlView.swift line 153 at r2 (raw file):

I'll put todo for that

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 42 at r2 (raw file):

is it the responsibility of tunnel to fetch exit ip.I don't think so.

ios/MullvadVPNTests/MockURLProtocol.swift line 12 at r2 (raw file):

if I'm gonna eliminate static variable in this particular part I need to go protocol-oriented for url-session and data task and so on that takes much efforts and bring more complexity.do you have any solution for this part?

ios/MullvadVPNTests/OutgoingConnectionProxy+Stub.swift line 12 at r2 (raw file):

fair enough

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 18 at r2 (raw file):

Done.

ios/MullvadVPNTests/OutgoingConnectionServiceTests.swift line 54 at r2 (raw file):

Done

Reviewable status: 5 of 16 files reviewed, 8 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

but task group does it parallely.
Yes but there are a couple of considerations at play here:

• We are only running 2 tasks
• We are doing network calls
• 3 lines of code is vastly easier to understand than 40 with a custom built enum

There's very likely more overhead to try to run 2 tasks in parallel rather than running them one after the other given how simple the calls are (and how fast iPhone processors are nowadays)
And since it's networking code, on a bad connection, it won't make any difference whether the tasks are running in parallel rather than in sequence.

I think we need to change it for better UX
Doing this will in the best case, not do anything, and in the worst case, make the whole system perform worse.
This is a good example of the Priority Inversion problem.
However, the swift async runtime should help us in those situations, hopefully. Happy to talk more about why that's a problematic design, but not in this comment.

ios/MullvadVPNTests/MockURLProtocol.swift line 12 at r2 (raw file):

Technically, the only part where we use that URLSession object is in OutgoingConnectionProxy where we do
let (data, response) = try await urlSession.data(from: url)

Ideally, we should have extracted that call to a single interface, then we wouldn't need all this mocking of URLProtocol and the complicated setup of tests.
That's a bit unfortunate, but it's not the end of the world.

I will create a cleanup janitor task instead, it's not critical.

Reviewed 1 of 4 files at r3.
Reviewable status: 6 of 16 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

Let's not get into micro optimizations.

Parallelization for IP lookup requests makes sense. This part looks right to me.

However I think that using async let binding to run two tasks in parallel and then collect the result would be simpler way to approach this.

async let ipv4Result = getIPv4(..)
async let ipv6Result = getIPv6(..)

OutgoingConnectionInfo(
  ipv4: try await ipv4Result, 
  ipv6: try? await ipv6Result
)

Reviewable status: 6 of 16 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

ios/MullvadVPN/View controllers/Tunnel/OutgoingConnectionService.swift line 30 at r2 (raw file):

accepted

ios/MullvadVPNTests/OutgoingConnectionProxyTests.swift line 34 at r2 (raw file):

Done.

Reviewable status: 6 of 16 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 83 at r2 (raw file):

It's a personal preferences.I'm fan of keeping protocols in individual files.let's know other's opinion.
@pronebird @rablador

Reviewed 5 of 9 files at r4.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @mojganii and @rablador)

Reviewed 2 of 4 files at r3, 8 of 9 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @mojganii, and @pronebird)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 83 at r2 (raw file):

I think it's fine to combine them if they're tightly coupled/associated with each other.

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 46 at r4 (raw file):

+1

Reviewable status: 13 of 19 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

ios/MullvadVPN/View controllers/Tunnel/TunnelViewControllerInteractor.swift line 46 at r4 (raw file):

Done.

Reviewed 6 of 6 files at r8, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pronebird)

Reviewed 1 of 4 files at r3, 3 of 6 files at r8, 2 of 3 files at r9, all commit messages.
Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @mojganii, @pronebird, and @rablador)

Reviewed 1 of 1 files at r5, 1 of 6 files at r8, 1 of 3 files at r9.
Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

ios/MullvadREST/RESTDefaults.swift line 17 at r9 (raw file):

    /// Exit ipV4  API hostname.
    public static let ipV4APIHostname = "ipv4.am.i.mullvad.net"

Nit: Maybe we should call it exitIPv4Hostname and exitIPv6Hostname instead because IPv4/6 API hostname sounds like the way we reach to api.mullvad.net. These are two different services however.

Reviewable status: 18 of 19 files reviewed, 4 unresolved discussions (waiting on @buggmagnet, @pronebird, and @rablador)

ios/MullvadREST/RESTDefaults.swift line 17 at r9 (raw file):

Done

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

this approach adds more complexity. Apple is using the same format for Codable

Code snippet:

public typealias Codable = Decodable & Encodable

Reviewable status: 18 of 19 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @mojganii, and @rablador)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

Extract the concrete value types

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 88 at r9 (raw file):

        } catch {
            throw error

do-catch is superficial as it doesn't do anything but rethrow an error. You can eliminate it to streamline the code.

Reviewed 3 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @mojganii)

Reviewed 1 of 9 files at r4, 1 of 6 files at r8, 1 of 3 files at r9, 2 of 2 files at r10, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

Reviewed 1 of 9 files at r4, 1 of 2 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

Reviewed 2 of 9 files at r4, 2 of 3 files at r9, 1 of 2 files at r10.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

To follow up on that. If the IPv{4|6}ConnectionData is a requirement of the OutgoingConnectionHandling protocol, then it's better to keep the connection data type around the protocol and not embed it into the concrete implementation to avoid cross-dependencies.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @mojganii)

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pronebird)

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 15 at r6 (raw file):

Done.

ios/MullvadVPN/GeneralAPIs/OutgoingConnectionProxy.swift line 88 at r9 (raw file):

Done.

ios/MullvadVPNTests/OutgoingConnectionProxy+Stub.swift line 12 at r6 (raw file):

done

Reviewed 7 of 7 files at r11.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewed 6 of 7 files at r11, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved